-
Notifications
You must be signed in to change notification settings - Fork 430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Added OAuth Support for Public APIs with TokenManager Integration #813
Conversation
@@ -214,6 +214,12 @@ public class Example { | |||
} | |||
``` | |||
|
|||
### OAuth Feature | |||
We are introducing Client Credentials Flow-based OAuth 2.0 authentication. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we mention the orgs auth is also in beta? since we changed back from preview?
@@ -1,12 +0,0 @@ | |||
package com.twilio.annotations; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did we remove this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous idea was to move our API from preview to beta and public as backend API moves, but we can not track that, so to keep simple we will have only Beta
|
||
@Override | ||
public String getAuthString() { | ||
return "Bearer " + token; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a null check here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call fetch token
final String credentials = this.username + ":" + this.password; | ||
final String encoded = Base64.getEncoder().encodeToString(credentials.getBytes(StandardCharsets.US_ASCII)); | ||
return "Basic " + encoded; | ||
if (username != null && password != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we convert username and password into basic auth strategy in any of the previous steps and just follow auth strategy based methods here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good suggestion, I think we might have to check if it will break existing customer who are using below configuration:
https://github.com/twilio/twilio-java/blob/main/advanced-examples/custom-http-client.md
@@ -59,7 +68,14 @@ protected TwilioRestClient(Builder b) { | |||
* @return Response object | |||
*/ | |||
public Response request(final Request request) { | |||
request.setAuth(username, password); | |||
if (username != null && password != null) { | |||
request.setAuth(username, password); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can move get auth string to individual auth strategy class and just fetch and set the value in request class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case if we moved username and password to authstrategy class, we can do that.
if(response != null) { | ||
int statusCode = response.getStatusCode(); | ||
if (statusCode == HTTP_STATUS_CODE_UNAUTHORIZED && authStrategy != null && EnumConstants.AuthType.TOKEN.equals(authStrategy.getAuthType())) { | ||
((TokenAuthStrategy)authStrategy).fetchToken(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving fetchtoken logic inside auth strategy class and just fetching the auth string would be more aligned to the concept of abstraction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed over call, this code is required for fetching token in case there is 401
Will be adding fetchToken in getAuthString() method as mentioned in previous comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Quality Gate failedFailed conditions See analysis details on SonarCloud Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
Feature
Added OAuth functionality for public APIs.
Checklist
If you have questions, please file a support ticket, or create a GitHub Issue in this repository.